ioemu: Fix PVFB backend to validate frontend's frame buffer description

A buggy or malicious frontend can describe its shared framebuffer to
the backend in a way that makes the backend map an arbitrary amount of
guest memory, malloc an arbitrarily large internal buffer, copy
arbitrary memory to that buffer, even beyond its end.  A domU running
a malicious frontend can abuse the former two for denial of service
attacks against dom0.  It can abuse the third to write arbitrary
backend memory.  It can abuse all three to terminate or crash the
backend.  Arbitrary code execution looks quite feasible.

In more detail (ignoring #ifdef CONFIG_STUBDOM code):

The frame buffer is described by the following parameters:
 * fb_len (size of shared framebuffer)
 * width, height, depth
 * row_stride, offset

fb_len is fixed on startup.  The frontend can modify the other
parameters by sending a XENFB_TYPE_RESIZE event.

xenfb_read_frontend_fb_config() limits fb_len according to backend
configuration parameter videoram (from xenstore), if present.  I
believe videoram is not present by default.

xenfb_map_fb() uses fb_len to map the frontend's framebuffer.

The frontend can make it map arbitrarily much, unless limited by the
videoram configuration parameter. This flaw always existed.

xenfb_register_console() and xenfb_on_fb_event() pass width, height,
depth and rowstride to QEMU's DisplayState object.  The object sets
itself up to work directly on the framebuffer (shared_buf true) if
parameters allow that.  Else it allocates an internal buffer of size
height * width * depth / 8 (shared_buf false).

The frontend can make it allocate arbitrarily much. This flaw always
existed.

xenfb_register_console() and xenfb_on_fb_event() pass width, height,
depth and rowstride to QEMU's DisplayState object.  The object sets
itself up to work directly on the framebuffer (shared_buf true) if
parameters allow that.  Else it allocates an internal buffer of size
height * width * depth / 8 (shared_buf false).

The frontend can make it allocate arbitrarily much. This flaw was
introduced by the move of PVFB into QEMU (xen-unstable cset 16220
ff).

xenfb_on_fb_event() uses width and height to clip the area of an
update event.  It then passes that area to xenfb_guest_copy().
xenfb_invalidate() passes the complete screen area to
xenfb_guest_copy().  xenfb_guest_copy() copies the argument area (x,
y, w, h) into the internal buffer, unless shared_buf is true.  This
copies h blocks of memory.  The i-th copy (counting from zero) copies
    w * depth / 8 bytes
from
    shared framebuffer + offset + (y + i) * row_stride + x * depth / 8
to
    internal buffer + (y + i) * ds->linesize + x * ds->depth / 8

where ds->linesize and ds->depth are parameters of the internal buffer
chosen by the backend.

This copy can be made to read from the shared framebuffer and write to
the internal buffer out of bounds.  I believe the frontend can abuse
this to write arbitrary backend memory.

The flaw in its current form was introduced by the move of PVFB into
QEMU (xen-unstable cset 16220 ff).  Before, the framebuffer was always
shared.

From: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Keir Fraser <keir.fraser@citrix.com>

diff --git a/tools/ioemu/hw/xenfb.c b/tools/ioemu/hw/xenfb.c
index 730712298077577e56bad7e60401307012ba6af4..32a94dced1a3c73c88758402b842a2820fd5e26b 100644 (file)
--- a/tools/ioemu/hw/xenfb.c
+++ b/tools/ioemu/hw/xenfb.c
@@ -29,8 +29,6 @@
 #define BTN_LEFT 0x110 /* from <linux/input.h> */
 #endif
 
-// FIXME defend against malicious frontend?
-
 struct xenfb;
 
 struct xenfb_device {
@@ -484,6 +482,68 @@ void xenfb_shutdown(struct xenfb *xenfb)
        free(xenfb);
 }
 
+static int xenfb_configure_fb(struct xenfb *xenfb, size_t fb_len_lim,
+                             int width, int height, int depth,
+                             size_t fb_len, int offset, int row_stride)
+{
+       size_t mfn_sz = sizeof(*((struct xenfb_page *)0)->pd);
+       size_t pd_len = sizeof(((struct xenfb_page *)0)->pd) / mfn_sz;
+       size_t fb_pages = pd_len * XC_PAGE_SIZE / mfn_sz;
+       size_t fb_len_max = fb_pages * XC_PAGE_SIZE;
+       int max_width, max_height;
+
+       if (fb_len_lim > fb_len_max) {
+               fprintf(stderr,
+                       "FB: fb size limit %zu exceeds %zu, corrected\n",
+                       fb_len_lim, fb_len_max);
+               fb_len_lim = fb_len_max;
+       }
+       if (fb_len > fb_len_lim) {
+               fprintf(stderr,
+                       "FB: frontend fb size %zu limited to %zu\n",
+                       fb_len, fb_len_lim);
+       }
+       if (depth != 8 && depth != 16 && depth != 24 && depth != 32) {
+               fprintf(stderr,
+                       "FB: can't handle frontend fb depth %d\n",
+                       depth);
+               return -1;
+       }
+       if (row_stride < 0 || row_stride > fb_len) {
+               fprintf(stderr,
+                       "FB: invalid frontend stride %d\n", row_stride);
+               return -1;
+       }
+       max_width = row_stride / (depth / 8);
+       if (width < 0 || width > max_width) {
+               fprintf(stderr,
+                       "FB: invalid frontend width %d limited to %d\n",
+                       width, max_width);
+               width = max_width;
+       }
+       if (offset < 0 || offset >= fb_len) {
+               fprintf(stderr,
+                       "FB: invalid frontend offset %d (max %zu)\n",
+                       offset, fb_len - 1);
+               return -1;
+       }
+       max_height = (fb_len - offset) / row_stride;
+       if (height < 0 || height > max_height) {
+               fprintf(stderr,
+                       "FB: invalid frontend height %d limited to %d\n",
+                       height, max_height);
+               height = max_height;
+       }
+       xenfb->fb_len = fb_len;
+       xenfb->row_stride = row_stride;
+       xenfb->depth = depth;
+       xenfb->width = width;
+       xenfb->height = height;
+       xenfb->offset = offset;
+       fprintf(stderr, "Framebuffer %dx%dx%d offset %d stride %d\n",
+               width, height, depth, offset, row_stride);
+       return 0;
+}
 
 static void xenfb_on_fb_event(struct xenfb *xenfb)
 {
@@ -514,16 +574,18 @@ static void xenfb_on_fb_event(struct xenfb *xenfb)
                            || h != event->update.height) {
                                fprintf(stderr, "%s bogus update clipped\n",
                                        xenfb->fb.nodename);
-                               break;
                        }
                        xenfb_guest_copy(xenfb, x, y, w, h);
                        break;
                case XENFB_TYPE_RESIZE:
-                       xenfb->width  = event->resize.width;
-                       xenfb->height = event->resize.height;
-                       xenfb->depth = event->resize.depth;
-                       xenfb->row_stride = event->resize.stride;
-                       xenfb->offset = event->resize.offset;
+                       if (xenfb_configure_fb(xenfb, xenfb->fb_len,
+                                              event->resize.width,
+                                              event->resize.height,
+                                              event->resize.depth,
+                                              xenfb->fb_len,
+                                              event->resize.offset,
+                                              event->resize.stride) < 0)
+                               break;
                        dpy_colourdepth(xenfb->ds, xenfb->depth);
                        dpy_resize(xenfb->ds, xenfb->width, xenfb->height, xenfb->row_stride);
                        if (xenfb->ds->shared_buf)
@@ -745,29 +807,18 @@ static int xenfb_read_frontend_fb_config(struct xenfb *xenfb) {
         xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "1");
         xenfb->refresh_period = -1;
 
-        /* TODO check for permitted ranges */
-        fb_page = xenfb->fb.page;
-        xenfb->depth = fb_page->depth;
-        xenfb->width = fb_page->width;
-        xenfb->height = fb_page->height;
-        /* TODO check for consistency with the above */
-        xenfb->fb_len = fb_page->mem_length;
-        xenfb->row_stride = fb_page->line_length;
-
-        /* Protect against hostile frontend, limit fb_len to max allowed */
         if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d",
                             &videoram) < 0)
                 videoram = 0;
-        videoram = videoram * 1024 * 1024;
-        if (videoram && xenfb->fb_len > videoram) {
-                fprintf(stderr, "Framebuffer requested length of %zd exceeded allowed %d\n",
-                        xenfb->fb_len, videoram);
-                xenfb->fb_len = videoram;
-                if (xenfb->row_stride * xenfb->height > xenfb->fb_len)
-                        xenfb->height = xenfb->fb_len / xenfb->row_stride;
-        }
-        fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n",
-                fb_page->depth, fb_page->width, fb_page->height, fb_page->line_length);
+       fb_page = xenfb->fb.page;
+       if (xenfb_configure_fb(xenfb, videoram * 1024 * 1024U,
+                              fb_page->width, fb_page->height, fb_page->depth,
+                              fb_page->mem_length, 0, fb_page->line_length)
+           < 0) {
+               errno = EINVAL;
+               return -1;
+       }
+
         if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0)
                return -1;